-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Refactor StableMIR #140643
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Refactor StableMIR #140643
Conversation
This comment has been minimized.
This comment has been minimized.
28f5ec1
to
a169e52
Compare
Sweet! I'll start the review tomorrow. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like the new name schema. One high level comment, I would prefer keeping StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. We could restrict the usage of internal items to be inside specific modules, maybe rustc_internal
and convert
modules. What do you think?
Ahh I agree. The only problem is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments. Let me know when you get to address them! Thanks
@rustbot author
Reminder, once the PR becomes ready for a review, use |
iiuc, we shouldn't use any internal items even like Or we just don't want to see something like |
I can live with I think we should avoid calling internal methods in |
☔ The latest upstream changes (presumably #141605) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we wrap TyCtxt<'tcx>
in a new SmirCtxt
? I'm not sure if this approach is correct or not.
c8f00a1
to
37370d5
Compare
This comment has been minimized.
This comment has been minimized.
pub fn predicates_of(&self, def_id: DefId) -> GenericPredicates<'tcx> { | ||
self.tcx.predicates_of(def_id) | ||
pub fn predicates_of( | ||
&self, | ||
def_id: DefId, | ||
) -> (Option<DefId>, Vec<(ty::PredicateKind<'tcx>, Span)>) { | ||
let ty::GenericPredicates { parent, predicates } = self.tcx.predicates_of(def_id); | ||
( | ||
parent, | ||
predicates | ||
.iter() | ||
.map(|(clause, span)| (clause.as_predicate().kind().skip_binder(), *span)) | ||
.collect(), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty unsure if this is a good approach but I can't figure out a perfect way🤷♂️
ummm pretty sure that I had something to say earlier but can’t quite recall it atm😅, so please let me know if you get any questions when reviewing, thanks! |
☔ The latest upstream changes (presumably #141942) made this pull request unmergeable. Please resolve the merge conflicts. |
37370d5
to
f88c65e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally got to do another pass. Just a few comments and I think we should be good.
use stable_mir::Error; | ||
use stable_mir::mir::Mutability; | ||
use stable_mir::ty::{Allocation, ProvenanceMap}; | ||
//! Internal memory allocator implementation for StableMIR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
alloc: &rustc_middle::mir::interpret::Allocation, | ||
alloc_range: AllocRange, | ||
tables: &mut Tables<'tcx>, | ||
) -> Allocation { | ||
) -> (Vec<Option<u8>>, Vec<(usize, AllocId)>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to create a rustc_smir
type here to encapsulate this?
☔ The latest upstream changes (presumably #142997) made this pull request unmergeable. Please resolve the merge conflicts. |
- rewrite all `SmirInterface` apis. - add `BridgeTys` to impl those associated types in `Bridge`. - move `**_def()` stuffs living in `impl Tables` from `rustc_internal` to `stable_mir`.
The previous `rustc_smir::alloc` had many direct calls to rustc queries. This commit splits it into two parts: `rustc_smir::alloc` and `stable_mir::alloc`. Following the same pattern as `SmirCtxt` and `SmirInterface`, the `rustc_smir::alloc` handles all direct interactions with rustc queries and performs the actual memory allocations, while the `stable_mir::alloc` is responsible for constructing stable components.
This commit removes the `Tables` field from `SmirCtxt`, since borrows of `tables` should only be managed by `SmirInterface`. This change prevents `SmirCtxt` from holding separate borrows and requires passing `tables` explicitly when needed. We use the `traits.rs` file to define traits that are used for encapsulating the associated functions in the rustc's internals. This is much easier to use and maintain than directly cramming everything into `SmirCtxt`.
note that this commit delete `convert/error.rs`, we would use `SmirError::from_internal` instead. **Unresolved questions:** - There are still a few direct calls to rustc's internals scattered across `impl Stable`s, but most of them appear to be relatively stable, e.g., `mir::interpret::ConstAllocation::inner(self)` and `mir::syntax::SwitchTargets::otherwise(self)`.
the only functionality of `Tables` is caching results. this commit moves calls to rustc queries from `Tables` to `SmirCtxt`.
define bridge types for `***Def`s. consolidate scattered `Tables` implementations into single inherent impl.
we should no longer keep `IndexMap` in `rustc_internal`, as we've decided to migrate `rustc_internal` to `stable_mir` under a feature.
add a new trait `InternalCx`, which defines the methods that are fine to call from `RustcInternal`. `RustcInternal::internal()` then takes a `impl InternalCx<'tcx>` instead of `TyCtxt<'tcx>`. make `tcx` in `SmirCtxt` public, since we need to pass it to `RustcInternal::internal()` in `SmirInterface`.
We want to keep StableMIR definitions and logic separate from any sort of conversion and usage of internal rustc code. So we bundle all unstable items that have no stability guarantees into `stable_mir::unstable`.
…explicit_predicates_of()`
…ine_discr_for_variant()`
f88c65e
to
a21cb79
Compare
a21cb79
to
56c942d
Compare
This PR refactors stable-mir according to the guidance in this doc. It reverses the dependency between
rustc_smir
andstable_mir
, makingrustc_smir
completely agnostic ofstable_mir
.Under the new architecture, the
rustc_smir
crate would retain direct access to rustc queries, whilestable_mir
should proxy all such requests throughrustc_smir
instead of accessing rustc's internals directly.stable_mir
would only be responsible for the conversion between internal and stable constructs.This PR mainly introduces these changes:
Since
rustc_smir
needs these stable types somewhere, using associated types is a good approach.This PR moves
Tables
fromSmirCtxt
to a newSmirContainer
struct, since mutable borrows oftables
should only be managed bySmirInterface
. This change preventsSmirCtxt
from holding separate borrows and requires passingtables
explicitly when needed:This PR introduces
SmirContainer
as a separate struct rather than bundling it into aSmirInterface
struct. This separation makes the architecture more modular and easier to reason about.We use this file to define traits that are used for encapsulating the associated functions in the rustc's internals. This is much easier to use and maintain than directly cramming everything into
SmirCtxt
. Here is a real-world use case:rustc_smir::alloc
The previous
rustc_smir::alloc
had many direct calls to rustc queries. This PR splits it into two parts:rustc_smir::alloc
andstable_mir::alloc
. Following the same pattern asSmirCtxt
andSmirInterface
, therustc_smir::alloc
handles all direct interactions with rustc queries and performs the actual memory allocations, while thestable_mir::alloc
is responsible for constructing stable components.convert/error.rs
We use
SmirError::from_internal
instead, since implementingStable
for these internal errors would be redundant—tables
is not actually used. If we later need to add something likeLayoutError
tostable_mir
, we could implement it as follows:Unresolved questions:
impl Stable
s, but most of them appear to be relatively stable, e.g.,mir::interpret::ConstAllocation::inner(self)
andmir::syntax::SwitchTargets::otherwise(self)
.r? @celinval